Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Remove UJSONResponse #1047

Merged
merged 4 commits into from
Nov 8, 2020
Merged

Remove UJSONResponse #1047

merged 4 commits into from
Nov 8, 2020

Conversation

JayH5
Copy link
Member

@JayH5 JayH5 commented Sep 6, 2020

Closes #901

It's easy enough to implement your own UJSONResponse response class if you need it:

from typing import Any

import ujson
from starlette.responses import JSONResponse

class UJSONResponse(JSONResponse):
    def render(self, content: Any) -> bytes:
        return ujson.dumps(content, ensure_ascii=False).encode("utf-8")

@euri10
Copy link
Member

euri10 commented Sep 6, 2020

I'm all in for that removal !
Not a blocker on my side but could we add the example you provided (or even better with orjson ) in docs/responses.md ?

@JayH5 JayH5 added the clean up Refinement to existing functionality label Sep 11, 2020
@JayH5
Copy link
Member Author

JayH5 commented Sep 12, 2020

@euri10 I wrote a bit about custom JSON serialization in responses.md. lmk what you think!

@euri10
Copy link
Member

euri10 commented Sep 12, 2020

@euri10 I wrote a bit about custom JSON serialization in responses.md. lmk what you think!

looks perfect ! 👍

@tomchristie
Copy link
Member

Fab yup!
Will need calling out in release notes and a median version bump.
Great stuff, thank you!

@erewok
Copy link
Contributor

erewok commented Nov 8, 2020 •
edited
Loading

@JayH5, this would have been a good one to include in the 0.14.0 release. Can we merge it and get it released after the fact as 0.14.1? It's pretty close to the "breaking-change" semver bump and with the recent release of 0.14.0 most users have probably not upgraded yet?

@JayH5
Copy link
Member Author

JayH5 commented Nov 8, 2020 •
edited
Loading

@erewok yeah sorry, I didn't want it to get in the way of all the fixes in "0.13.9" and then when we switched to 0.14 I forgot about it.

Happy to squeeze it in if you want to do that? I won't be able to organise a release right now (haven't actually done one before), but if you want to merge this and do one be my guest.

@erewok
Copy link
Contributor

erewok commented Nov 8, 2020

@JayH5, no worries!

I do think we could take advantage of the "breaking change" version bump to merge this (and probably any other test or documentation changes that are easily to include) and make another release.

Would you be up for merging this and issuing a release? (For reference: https://www.python-httpx.org/contributing/#releasing)

@JayH5
Copy link
Member Author

JayH5 commented Nov 8, 2020

(and probably any other test or documentation changes that are easily to include)

I don't see any other obvious candidate PRs. Will give this a go.

@JayH5 JayH5 merged commit 99b3778 into master Nov 8, 2020
@JayH5 JayH5 deleted the rm-ujsonresponse branch November 8, 2020 20:49
@yareyaredesuyo yareyaredesuyo mentioned this pull request Dec 9, 2020
Merged
@Kludex Kludex mentioned this pull request Feb 4, 2021
Merged
@mweinelt mweinelt mentioned this pull request Feb 11, 2021
Merged
10 tasks
@ycd ycd mentioned this pull request Feb 16, 2021
Closed
@dependabot dependabot bot mentioned this pull request Mar 18, 2021
Closed
mweinelt added a commit to mweinelt/nixpkgs that referenced this pull request Mar 20, 2021
@mweinelt
python3Packages.starlette: 0.13.8 -> 0.14.2
f899f3a 
- ujson was dropped in encode/starlette#1047
- test_debug_html was fixed in encode/starlette#1132
@gasabr gasabr mentioned this pull request Jun 3, 2021
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@euri10 euri10 euri10 approved these changes

Assignees
No one assigned
Labels
clean up Refinement to existing functionality
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Consider removing UJSONResponse from core
4 participants
@JayH5 @euri10 @tomchristie @erewok